fix(providers): init options.hooks before merging node hooks#1235
fix(providers): init options.hooks before merging node hooks#1235mayckol wants to merge 1 commit intocoleam00:devfrom
Conversation
applyNodeConfig is called twice in sendQuery: once against a throwaway
`{} as Options` to extract warnings, then again against the real Options
produced by buildBaseClaudeOptions (which seeds the PostToolUse capture
hook). The first call crashed with
TypeError: undefined is not an object (evaluating
'options.hooks[event] = matchers')
whenever a node declared `hooks:` in its nodeConfig, because the merge
loop assigned directly into `options.hooks` without ensuring it existed.
This blocked every DAG workflow that relied on per-node hooks (e.g.
archon-architect's analyze node) at the first AI layer.
Initialize `options.hooks = {}` before the merge loop so the assignment
is safe regardless of whether the caller pre-populated the hooks map.
Also add a regression test that exercises sendQuery with
`nodeConfig.hooks` and asserts both the node hook and the provider's
PostToolUse capture hook land on the SDK options.
📝 WalkthroughWalkthroughThis change adds a regression test for hook propagation in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Thanks for diagnosing and fixing this, @mayckol — closing because the fix landed independently via #1177 yesterday. Both PRs root-cause identically to the warning-extraction pathway calling `applyNodeConfig` against a throwaway `{} as Options`, and both fix it the same way (`if (!options.hooks) options.hooks = {}` before the merge loop). Dev currently has: ```ts which is functionally the same as your patch — so the code change is a no-op on current `dev`. However, your regression test is the piece #1177 didn't include and would be valuable to land on its own. The test asserts both sides of the invariant (`nodeConfig.hooks` lands on SDK options AND the provider's own PostToolUse capture hook is preserved), which is exactly the kind of lock-in that would have caught this class of bug earlier and would guard against any future refactor in `applyNodeConfig` dropping one or the other. If you'd like, open a small test-only follow-up PR with just the `'passes hooks to SDK via nodeConfig without crashing on warning extraction'` test from this branch. The code in `applyNodeConfig` is already what your test expects, so the test lands cleanly and locks the invariant down. Happy to merge that fast. Thanks again for catching this independently — glad the `archon-architect` workflow now works for you. |
2 participants
fix(providers): init options.hooks before merging node hooks
Summary
applyNodeConfigis called twice insendQuery:{} as Optionsto extract provider warnings (line 898).Optionsbuilt bybuildBaseClaudeOptions, which pre-seedshookswith the PostToolUse tool-capture hook (line 941).The first call crashed whenever a node declared
hooks:in itsnodeConfig, because the merge loop wrote directly intooptions.hookswithout checking it existed. This blocks every DAG workflow that uses per-node hooks (e.g. theanalyzenode ofarchon-architect) at the very first AI layer.Error reproduced
Before:
After:
Reproduction: run any DAG workflow whose first AI node defines
hooks:in its YAML. The stockarchon-architectworkflow (analyzenode withPreToolUse/PostToolUsehooks) hits it deterministically —scan-metricscompletes in ~100 ms, thenanalyzefails instantly with the TypeError and the remaining 5 nodes are skipped.Fix
Initialize
options.hooks = {}before the merge loop so the subsequentoptions.hooks[event] = ...assignments are safe regardless of whether the caller pre-populated the hooks map. The second call path (line 941) is unaffected because itsoptions.hooksis already populated bybuildBaseClaudeOptions.Test plan
provider.test.tsexercisessendQuerywithnodeConfig.hooksand asserts both the node hook and the provider's PostToolUse capture hook land on the SDK options. Fails ondevwithout the patch, passes with it.bun run type-check— all packages passbun run lint— cleanbun run format:check— cleanbun run test— all packages pass (64 tests in@archon/providersalone)archon-architectend-to-end after the patch:analyzenode streams normally for minutes instead of crashing at ~1 ms.